Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement "form" check box control #29712

Merged
merged 4 commits into from
Sep 13, 2024
Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Sep 6, 2024

Continuation of #29643.

2024-09-06.11-39-32.mp4

@bdach bdach force-pushed the editor/setup-screen-checkbox branch from 721e676 to 2c19b79 Compare September 6, 2024 09:45
@peppy
Copy link
Member

peppy commented Sep 10, 2024

A bit concerned that the checkbox is so far away from the actual text (rather than being next to it on the left). One would hope these would only be used in fixed-width displays, but I get the feeling that's not the case?

Like in the video when you're clicking to toggle my brain was very confused as to what was going on:

  • Text changing on a checkbox? This seems weird.
  • Where's the checkbo-- oh there it is over to the right.

@bdach
Copy link
Collaborator Author

bdach commented Sep 10, 2024

One would hope these would only be used in fixed-width displays

Not sure what you mean by "fixed width" in this context. I generally don't intend this component to be used or even usable in contexts where it's allowed to autosize width wise. The design only makes sense in vertically flowing forms where every single field is of equal width.

@peppy
Copy link
Member

peppy commented Sep 11, 2024

Not sure what you mean by "fixed width" in this context

I just mean we probably want to avoid scenarios where this distance is more than 50px or so

2024-09-12 01 47 06@2x

@bdach
Copy link
Collaborator Author

bdach commented Sep 11, 2024

Hmm okay... I'm not super sure I can guarantee that.

I'll consider options. Maybe the nub can be moved to the left of text instead.

@peppy
Copy link
Member

peppy commented Sep 12, 2024

Also the text is still completely weird to me. Like there's a checkbox title, then a customised yes/no line which – at least in the two provided examples – adds nothing but confusion to my brain. Like, the checkbox is there to imply state, so adding text just throws me off. I've never seen this done anywhere and I'm not sure why we're doing it here.

It would also mean more localisation work for no added benefit.

@bdach
Copy link
Collaborator Author

bdach commented Sep 12, 2024

The customised yes/no thing is something I pulled from a design sketch in https://www.figma.com/file/IjrU0u7MFdgmxDofqTlsZb/UI-Client-Settings?node-id=0%3A1. Some check boxes there use different text depending on context. I would not mind removing the customisability of that if it helps speed things up.

@arflyte
Copy link

arflyte commented Sep 12, 2024

I'd imagined for the long screen, it'd be in two columns. As for the idea of the changing text is to provide more context of that it's doing for certain things that has two options other than 'yes' or 'no' like "Classic" or "Modern" for example.

If we're not happy with the direction, best we keep the current style for the moment.

As for the switch location, after discussion with Nanaya today, probably it's best for me to explore moving the button to the other side.

@peppy
Copy link
Member

peppy commented Sep 12, 2024

like "Classic" or "Modern" for example.

I don't foresee this ever being used. That's not how we design boolean options.

@bdach
Copy link
Collaborator Author

bdach commented Sep 13, 2024

I pushed a half-hearted commit to attempt to address the feedback above but I'm very unsure about the end result.

2024-09-13.09-58-57.mp4

Maybe could be better if the nub didn't change width, but all other nubs across the game do that. Dunno.

@peppy
Copy link
Member

peppy commented Sep 13, 2024

Functional but looks pretty bad. I think having it on the right is better but we need to limit width of all usages to ensure it doesn't become screen-wide. Arguably we should be doing this in most places with form content (see this egregious example):

osu! 2024-09-13 at 10 19 06

First run setup does it (correctly) by limiting the full dialog size:

osu! 2024-09-13 at 10 19 38

but also just limiting the content width and centring it inside the containing box, or making it 2 column wide (more risky for handling 4:3 squashing though).


Put another way: if you're okay to make sure we limit the width of the editor form display, then just revert to the previous design, remove the localisation of the enabled/disabled state text, and make the test scene also limited width so it doesn't look silly and we should be good to go.

To better reflect what the widths should be in actual usage.
@peppy peppy self-requested a review September 13, 2024 17:20
@peppy peppy merged commit 5ea7061 into ppy:master Sep 13, 2024
11 of 13 checks passed
@bdach bdach deleted the editor/setup-screen-checkbox branch September 17, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants